Skip to content

feat(gateway): serve an operator-supplied certificate, and fix the healthcheck - #6859

Merged
otavio merged 2 commits into
masterfrom
feat/gateway-tls-cert-files
Aug 5, 2026
Merged

feat(gateway): serve an operator-supplied certificate, and fix the healthcheck#6859
otavio merged 2 commits into
masterfrom
feat/gateway-tls-cert-files

Conversation

@geovannewashington

@geovannewashington geovannewashington commented Aug 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

What

Two independent changes to the gateway, one commit each.

fix(gateway) makes the container healthcheck answer on a site this proxy
actually serves. feat(gateway) adds SHELLHUB_TLS_CERT_FILE and
SHELLHUB_TLS_KEY_FILE, which name a certificate to serve as-is instead of
obtaining one over ACME.

The fix stands alone and could be merged on its own. The feature cannot be
exercised without it, which is why they travel together.

Why

The healthcheck. It probed http://gateway/healthcheck. That name is the
container's own name on the Docker network, never one of the site addresses
the proxy serves, so Caddy answered it the way it answers any name it does
not recognise: with an empty 200. The probe reported healthy whatever state
the stack was in, and a made-up path got exactly the same answer. With
SHELLHUB_AUTO_SSL on it inverts: automatic HTTPS redirects that name to
itself over HTTPS, where no certificate can match it, so the handshake
fails and the container never becomes healthy. Nothing in the compose file
gates on gateway health, so today this surfaces only as an unhealthy label
on a container that plainly works.

The certificate. The gateway had one way to get a certificate: ask a
public CA. That leaves no way to run HTTPS on a name no public authority
will sign, which is every internal hostname and every domain a deployment
does not own. Against such a name SHELLHUB_AUTO_SSL cannot succeed, and
Caddy retries for as long as the process runs. This is what an on-prem
deployment behind a corporate CA needs, and what a local demo needs to
serve a branded domain over trusted HTTPS.

Changes

fix(gateway)

  • Caddyfile.tmpl: a http://healthcheck.internal site answering
    /healthz. The scheme is explicit so automatic HTTPS cannot redirect it,
    and the name is its own so it shares port 80 with the main site without
    either matching the other's requests.
  • docker-compose.yml: extra_hosts resolves that name to 127.0.0.1
    inside the container, and the probe targets it. The name is repeated in
    both files, so the two have to move together.
  • Nothing is shadowed: /healthcheck on the real domain still routes to
    the API, and no client addresses healthcheck.internal. It is reachable
    from outside on the published port by sending that Host header, which
    returns the string ok and nothing else.
  • Whether the API answers is left to the API's own healthcheck, which
    server and ui already have.

feat(gateway)

  • config.go: the two variables, validated required_with each other.
    Both or neither, checked even with TLS off: a half-set pair is a typo in
    every case, and the alternative to refusing it at startup is quietly
    asking a public CA instead of serving what was supplied.
  • Caddyfile.tmpl: a tls <cert> <key> directive when both are set. This
    is also what keeps Caddy off ACME, since a site with no tls directive
    gets the default, which is to go and ask for one. Only the main site is
    affected; the web endpoints wildcard is still obtained over DNS-01.
  • docker-compose.yml passes both through; .env and the gateway README
    document them. The paths are read inside the gateway container, so the
    certificate has to be mounted in.

No behaviour changes when the variables are unset, which is every existing
deployment.

Testing

Beyond the unit tests, this was exercised end to end with a certificate no
public CA would ever sign, on the release gateway image and on this build.

  1. Issue one and trust it locally:

    mkcert -install
mkcert shellhub.acme-robotics.com

  2. Point the name at the host: add it to /etc/hosts as 127.0.0.1.

  3. Bring the stack up with the certificate mounted in, via a compose
    override that bind-mounts the pair into the gateway:

    SHELLHUB_DOMAIN=shellhub.acme-robotics.com
SHELLHUB_AUTO_SSL=true
SHELLHUB_TLS_CERT_FILE=/etc/shellhub/certs/fullchain.pem
SHELLHUB_TLS_KEY_FILE=/etc/shellhub/certs/privkey.pem

  4. Verify the chain, with no -k:

    curl -v https://shellhub.acme-robotics.com/

    Expect SSL certificate verify ok, and a 308 from the http:// address.
    A browser shows a padlock and reports a secure context, which is what
    WebAuthn and clipboard access need.

  5. Verify the healthcheck:

    docker inspect --format '{{.State.Health.Status}}' <gateway>

    Expect healthy with SHELLHUB_AUTO_SSL on and off, and with
    SHELLHUB_PROXY on, since the listener wrappers apply to every server.
    On master it is unhealthy with TLS on, and healthy-by-accident with
    TLS off: wget http://gateway/anything-at-all returns the same empty
    200 the probe was reading.

  6. Half-set pair check: set only SHELLHUB_TLS_CERT_FILE and the gateway
    refuses to start with a validation error, rather than silently asking
    Let's Encrypt for a name it cannot prove.

@geovannewashington geovannewashington self-assigned this Aug 4, 2026
@geovannewashington
geovannewashington requested a review from a team as a code owner August 4, 2026 18:32
@geovannewashington geovannewashington added status/needs-review go Pull requests that update Go code area/gateway labels Aug 4, 2026
@github-actions

github-actions Bot commented Aug 4, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude finished @geovannewashington's task in 2m 35s —— View job

Code Review Complete

Reviewed 6 files across code quality, security, testing, language patterns,
and architecture — no new issues found. The code looks good as-is.

A few things I verified while reviewing, for the record:

  • The go.mod/go.sum go-envconfig change that shows up in a raw git diff against origin/master is base divergence (master bumped it after this branch's base), not part of this PR — it's correctly absent from the changed-files set.
  • TestCaddyfileServesTheSuppliedCertificate's NotContains "tls " assertion holds because the "auto ssl" config has WebEndpoints=false and no proxy protocol, so nothing else emits a tls token.
  • The required_with pair enforces both-or-neither correctly (empty strings read as absent, so both-empty passes, half-set fails).
  • http://:8081 is safe under SHELLHUB_AUTO_SSL=true: the explicit http:// scheme suppresses the automatic-HTTPS redirect, and a port-only address gives ACME no name to chase. TestCaddyfileAdapts runs the real Caddy adapter over every config shape including the new block.

If you push additional changes and want a new review, tag
@shellhub-io/admin and a team member can trigger it.

otavio
otavio previously approved these changes Aug 4, 2026
View reviewed changes
@geovannewashington
geovannewashington force-pushed the feat/gateway-tls-cert-files branch from d6bc1b5 to b022337 Compare August 4, 2026 20:45
@otavio

otavio commented Aug 4, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Aug 4, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude finished @otavio's task in 1m 42s —— View job

Code Review Complete

Reviewed 6 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is.

A few things verified while reviewing, for the record:

  • required_with pairing (gateway/config.go:66-67) enforces both-or-neither correctly: only-cert fails on TLSKeyFile's rule, only-key fails on TLSCertFile's rule, both-empty and both-set pass. Half-set refusal at startup works as described.
  • TLS conditional (gateway/Caddyfile.tmpl:73) — and .TLS .TLSCertFile .TLSKeyFile, with .TLS = EnableAutoSSL. The "certificate only applies when AUTO_SSL is on" behavior is documented in .env, and a site with no tls directive still falls back to the default ACME path, so no existing deployment changes.
  • Healthcheck site (gateway/Caddyfile.tmpl:200) — explicit http:// scheme suppresses the automatic-HTTPS redirect, and the distinct healthcheck.internal name shares port 80 without shadowing the main site's request matching. /healthcheck on the real domain still routes to the API; nothing addresses healthcheck.internal as a client. TestHealthcheckAnswersOnEveryConfiguration pins its presence across all config shapes.
  • docker-compose.yml — extra_hosts maps healthcheck.internal to 127.0.0.1 and the probe targets /healthz, matching the template. The name is repeated in both files as the PR notes.
  • No secrets introduced; cert/key are file paths read inside the container, not embedded material.

If you push additional changes and want a new review, tag @shellhub-io/admin and a team member can trigger it.

@geovannewashington
fix(gateway): stop the healthcheck asking for a name nothing serves
4d7d1b9 
The container healthcheck probed http://gateway/healthcheck. That name is
the container's own name on the Docker network, never one of the site
addresses this proxy serves, so Caddy answered it the way it answers any
name it does not recognise: with an empty 200. The probe called the
gateway healthy whatever state the stack was in, and a made-up path got
exactly the same answer as the real one.

With SHELLHUB_AUTO_SSL on it stops being harmless. Automatic HTTPS
redirects that name to itself over HTTPS, where no certificate can match
it, so the handshake fails and the container never becomes healthy. Every
deployment serving TLS has this today. Nothing acts on the status, so it
surfaces only as an unhealthy label on a container that plainly works.

It now answers on healthcheck.internal, a name of its own, written http://
so automatic HTTPS cannot redirect it. Compose resolves that name to
127.0.0.1 inside the container, so it shares port 80 with the main site
without either matching the other's requests. Whether the API answers is
left to the API's own healthcheck, which server and ui already have.
@geovannewashington
geovannewashington force-pushed the feat/gateway-tls-cert-files branch from b022337 to e69c3f9 Compare August 5, 2026 18:20
@otavio

otavio commented Aug 5, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Aug 5, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude finished @otavio's task in 4m 47s —— View job

Code Review Complete

Reviewed 6 files across code quality, security, testing, language patterns, and architecture — no new blocking issues found. The code looks good as-is.

Verified during review:

  • required_with pairing (gateway/config.go:66-67) — enforces both-or-neither exactly: only-cert and only-key each fail validation, both-empty and both-set pass.
  • TLS conditional (gateway/Caddyfile.tmpl:73) — and .TLS .TLSCertFile .TLSKeyFile is true only when AUTO_SSL is on and both paths are non-empty (empty string is falsy in text/template). A site with no tls directive still falls back to the default ACME path, so no existing deployment changes.
  • NotContains "tls " in TestCaddyfileServesTheSuppliedCertificate — holds: the only tls (trailing-space) token in the template is the gated cert directive; the auto ssl config leaves it out, and caddyfile.Format only reflows indentation.
  • Healthcheck site — http://healthcheck.internal is a separate block that can't shadow the main site's route ordering; explicit http:// suppresses the auto-HTTPS redirect; returns only a static "ok", exposing nothing sensitive even though it's reachable via Host header on the published port. Works under proxy-protocol since that listener wrapper's default policy doesn't require a PROXY preamble.
  • Cross-repo — cloud/ has no duplicate Caddyfile; its gateway overlays only override image/environment, so they inherit the new extra_hosts + probe. No depends_on: service_healthy targets the gateway, and no stale references to the old http://gateway/healthcheck remain. The new TLS vars propagate to enterprise/cloud via the shared base .env.

Additional notes

  • Minor, non-blocking: .env documents SHELLHUB_TLS_CERT_FILE/SHELLHUB_TLS_KEY_FILE (including the "only used when AUTO_SSL is on" detail), but the gateway README.md TLS section wasn't updated to match. Worth syncing for discoverability, but not a defect.

If you push additional changes and want a new review, tag @shellhub-io/admin and a team member can trigger it.
· feat/gateway-tls-cert-files

@geovannewashington
feat(gateway): serve a certificate supplied by the operator
d2fb447 
The gateway had one way to get a certificate: ask a public CA over ACME.
That leaves no way to run HTTPS on a name no public authority will sign,
which is every internal hostname and every domain a deployment does not
own. SHELLHUB_AUTO_SSL could only mean "go and obtain one", and against
such a name it fails and keeps retrying for as long as the process runs.

SHELLHUB_TLS_CERT_FILE and SHELLHUB_TLS_KEY_FILE name a certificate and
its key to serve as they are. Naming them also stops the ACME request,
because a site with no tls directive gets Caddy's default, which is to go
and ask for one. The paths are read inside the gateway container, so the
certificate has to be mounted in.

Both or neither, checked even with TLS off: a half-set pair is a typo in
every case, and the alternative to refusing it at startup is quietly
asking a public CA instead of serving what was supplied.
@geovannewashington
geovannewashington force-pushed the feat/gateway-tls-cert-files branch from e69c3f9 to d2fb447 Compare August 5, 2026 18:59
@otavio
otavio merged commit d372b85 into master Aug 5, 2026
38 checks passed
@otavio
otavio deleted the feat/gateway-tls-cert-files branch August 5, 2026 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@otavio otavio otavio left review comments

Assignees

@geovannewashington geovannewashington

Labels

area/gateway go Pull requests that update Go code status/needs-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@geovannewashington @otavio